Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding mvapich support #235

Conversation

j34ni
Copy link
Contributor

@j34ni j34ni commented Oct 23, 2024

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

Here I tried to add MVAPIch support (latest version, regardless of the netmod, for now), keeping version 1.14.4 instead of moving to 1.14.5.

@j34ni
Copy link
Contributor Author

j34ni commented Oct 23, 2024

@conda-forge-admin, please rerender

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe/meta.yaml) and found it was in an excellent condition.

@j34ni j34ni changed the title Update to hdf5 1.14.5 and add mvapich support Update to hdf5 1.14.5 and add mvapich support - Work in progress Oct 23, 2024
@hmaarrfk hmaarrfk marked this pull request as draft October 23, 2024 16:32
@hmaarrfk
Copy link
Contributor

ping again if/when ready or you need help.

@j34ni
Copy link
Contributor Author

j34ni commented Oct 23, 2024

@hmaarrfk Thanks, I will get in touch very soon then

@j34ni
Copy link
Contributor Author

j34ni commented Oct 23, 2024

@hmaarrfk I think I got it working at least for linux-64/osx64/win, but there must be a need to update the patches for other platforms (osx-arm64, etc.)
Is this something you can help with?

@hmaarrfk
Copy link
Contributor

unfortunately i do'nt have time. please see the other PRs where others have been working on something similar.

@j34ni j34ni changed the title Update to hdf5 1.14.5 and add mvapich support - Work in progress Keeping version hdf5 1.14.4 and adding mvapich support - Work in progress Oct 25, 2024
@j34ni
Copy link
Contributor Author

j34ni commented Oct 26, 2024

@hmaarrfk Is it all good now?

@j34ni j34ni changed the title Keeping version hdf5 1.14.4 and adding mvapich support - Work in progress Keeping version hdf5 1.14.4 and adding mvapich support Oct 26, 2024
@j34ni j34ni marked this pull request as ready for review October 26, 2024 07:14
@@ -0,0 +1,6 @@
#include <stdio.h>
int main(void) {
printf("Replacement test for t_filters_parallel\n");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do these test files do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hmaarrfk The t_filters_parallel test suite ensures the correct application and integrity of HDF5 filters, such as compression, in a parallel I/O context. The t_pmulti_dset test suite verifies the proper creation and I/O operations on multiple datasets in parallel. We had to disable these tests for MVAPICH due to specific failures for a couple of them, likely related to resource constraints in the testing environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anything else needed from my side before this PR can be merged?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add this statement as a comment in the source

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@j34ni j34ni changed the title Keeping version hdf5 1.14.4 and adding mvapich support Adding mvapich support Oct 30, 2024
recipe/build.sh Outdated Show resolved Hide resolved
@hmaarrfk
Copy link
Contributor

hmaarrfk commented Nov 4, 2024

@conda-forge-admin please rerender

Please remember to bump the build number next time too!

Sorry i had to review how you "fixed" osx-arm. it seems you reverted to the older version

@hmaarrfk
Copy link
Contributor

hmaarrfk commented Nov 5, 2024

why did it magically start to fail?

@hmaarrfk
Copy link
Contributor

hmaarrfk commented Nov 5, 2024

you may unfortunately have to rebase: #239

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe/meta.yaml) and found it was in an excellent condition.

@hmaarrfk
Copy link
Contributor

hmaarrfk commented Nov 5, 2024

i mostly just wanted to trigger a build

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I was trying to look for recipes to lint for you, but it appears we have a merge conflict. Please try to merge or rebase with the base branch to resolve this conflict.

Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.

@j34ni
Copy link
Contributor Author

j34ni commented Nov 5, 2024

@conda-forge-admin, please rerender

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe/meta.yaml) and found it was in an excellent condition.

@hmaarrfk
Copy link
Contributor

hmaarrfk commented Nov 5, 2024

hint: you must bump the build number again

@j34ni
Copy link
Contributor Author

j34ni commented Nov 5, 2024

@hmaarrfk It seems that the build for the HDF5 package on Linux with MVAPICH was failing due to missing symbols related to libacl (which for some reason was not needed until now?!), and adding this package on the host resolved the issue, allowing the build to proceed successfully.
However there is now an issue with win64 and '".scripts\create_conda_build_artifacts.bat"' is not recognized as an internal or external command: did I mess up something else?

@hmaarrfk
Copy link
Contributor

hmaarrfk commented Nov 5, 2024

rebasing is etter than merging.

when you rebase, drop all the MNT commits from rerendering

@j34ni
Copy link
Contributor Author

j34ni commented Nov 5, 2024

@conda-forge-admin, please rerender

@j34ni
Copy link
Contributor Author

j34ni commented Nov 5, 2024

@hmaarrfk libacl is not required at runtime, only during the build, so I guess we can add;

  ignore_run_exports:
    - libacl

@hmaarrfk
Copy link
Contributor

hmaarrfk commented Nov 5, 2024

@hmaarrfk libacl is not required at runtime, only during the build, so I guess we can add;

  ignore_run_exports:
    - libacl

yes.

@hmaarrfk
Copy link
Contributor

hmaarrfk commented Nov 5, 2024

please add a comment referring to this PR.

@j34ni
Copy link
Contributor Author

j34ni commented Nov 5, 2024

Or should I simply add it myself (I did not want to interrupt the build on Azure, which is now becoming all green)?

@hmaarrfk
Copy link
Contributor

hmaarrfk commented Nov 5, 2024

choose to interupt at your leisure or to let pass, but given this discussion, i'll wait until you you add the ignore run export and comment before we merge into main

@hmaarrfk
Copy link
Contributor

hmaarrfk commented Nov 5, 2024

if you have the git skills, it would be good to rebase this into a single commit + and a single rerender

Combined changes:
- Removed valgrind
- Update recipe/conda_build_config.yaml
  - Co-authored-by: Mark Harfouche <[email protected]>
- Moved --enable-tests=no
- Use *_FOR_BUILD for MVAPIch as for OpenMPI
- Revert to hdf5 version 1.14.4 and add mvapich support
- Restored source for 1.14.4
- Skip t_pmulti_dset for MVAPIch
- Restored hdf5_cv_szlib_can_encode=yes
- Skip t_filters_parallel for MVAPIch
- Including comment regarding t_filters_parallel and t_pmulti_dset tests
- remove unecessary space
- bump build number
- do not store artifacts
- Capture environment configuration
- Update dummy_t_filters_parallel.c
- Reverting to the commit which previously built
- remove Libs.private from hdf5.pc on windows
- Add reference
- Address dynamic dependencies ensuring path alignment avoiding extraneous conflicts
- Conditionally include libacl for Linux systems only
- Bumped up build number
- Restored .azure-pipelines/azure-pipelines-win.yml
- Add ignore_run_exports to meta.yaml
@j34ni j34ni force-pushed the Update_to_hdf5-1.14.5_and_add_mvapich_support branch from aa0c7c9 to c7543bd Compare November 5, 2024 14:52
@j34ni
Copy link
Contributor Author

j34ni commented Nov 5, 2024

@conda-forge-admin, please rerender

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I was trying to look for recipes to lint for you, but it appears we have a merge conflict. Please try to merge or rebase with the base branch to resolve this conflict.

Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe/meta.yaml) and found it was in an excellent condition.

@hmaarrfk hmaarrfk added the automerge Merge the PR when CI passes label Nov 5, 2024
@github-actions github-actions bot merged commit 0242148 into conda-forge:main Nov 5, 2024
23 checks passed
Copy link
Contributor

github-actions bot commented Nov 5, 2024

Hi! This is the friendly conda-forge automerge bot!

I considered the following status checks when analyzing this PR:

  • linter: passed
  • azure: passed

Thus the PR was passing and merged! Have a great day!

@j34ni
Copy link
Contributor Author

j34ni commented Nov 5, 2024

Thanks @hmaarrfk
Now I just have to continue with NetCDF, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge the PR when CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants